Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce response decoder interface #49

Merged
merged 1 commit into from
Nov 25, 2018

Conversation

maxbeutel
Copy link
Contributor

I use Sling to get data from JSON APIs, soon I will have the use case to receive data from XML APIs, too. Hence I suggest a way that allows decoding responses from arbitrary sources:

  • Introduce a ResponseDecoder interface that abstracts away the actual decoding
  • Provide a default jsonDecoder that does what the library is doing at the moment: decoding from JSON (keep bc)
  • I also added a new test case, not sure if this is sufficient.
  • Documentation is missing for the new feature, I need to add this.

Is there a chance of getting this merged?

@maxbeutel
Copy link
Contributor Author

@dghubble would it be possible to get feedback on the PR, it would be a very useful feature in my case. Thanks! :-)

Just noticed the build is failing for G 1.8, need to look into that.

Copy link
Owner

@dghubble dghubble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems agreeable to me. I've left some nitpicks about how its documented.

Also, you can rebase (and squash if needed after fixes) to resolve the CI failure.

sling.go Outdated Show resolved Hide resolved
response.go Outdated Show resolved Hide resolved
response.go Show resolved Hide resolved
response.go Outdated Show resolved Hide resolved
response.go Show resolved Hide resolved
response.go Outdated Show resolved Hide resolved
response.go Outdated Show resolved Hide resolved
@maxbeutel
Copy link
Contributor Author

Thanks for the feedback and corrections! I updated the PR & rebased.

@codyaray
Copy link

i bet this would come in handy ;) #47

@maxbeutel
Copy link
Contributor Author

@dghubble friendly bump :-) good to merge now?

@dghubble dghubble merged commit c3195df into dghubble:master Nov 25, 2018
@wojteninho
Copy link

@dghubble may I ask to create a new release including above changes? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants